fix(session-flow): carry amendment bullets below the predecessor's Opening ask - #4164

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Claude finished @kyle-sexton's task in 53s —— View job

Security review — /review:security-review

• Gather context (PR diff via gh pr diff 4164, read changed files)
• Apply skip gate
• Report result

Skip gate hit: no security-relevant surface.

This PR's entire diff is:

• plugins/session-flow/scripts/save_point.py — lines 340–346 add a helper _ends_opening_ask() reusing the existing BULLET_RE (a bounded, non-backtracking regex — no ReDoS concern), and lines 590–596 / lines 1098–1104 swap it in for the old line.startswith("**") terminator in two markdown section-skip loops.
• plugins/session-flow/scripts/tests/test_save_point.py — one new pytest case covering the fix.
• plugins/session-flow/CHANGELOG.md / .claude-plugin/plugin.json — version bump and changelog entry only.

This is a local text-parsing change to how the save_point.py new command delimits a markdown section while assembling one locally-authored handoff file from another (both produced and consumed by the same toolchain, not external/untrusted input). There's no authentication, authorization, secret handling, command execution, path construction, or trust-boundary crossing anywhere in the diff — just where a copy loop stops skipping lines. No CI/Actions workflow files are touched.

No security findings. Per the skill's high-signal bar, nothing further is reported.

Last security-reviewed head: e0d963d86371f09beafc7c436b2924b2d54e1b3c. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

Claude finished @kyle-sexton's task in 3m 27s —— View job

Code review (/review:code-review)

Reviewed the diff against origin/main (4 files, +42/-3). This is a real, well-scoped bug fix with a reproducing regression test; the fix itself (_ends_opening_ask, reused at both call sites) is sound and matches the stated intent of collapsing two boundary definitions into one.

Finding

The fix leaves a third, still-inconsistent definition of the same boundary in _check_original_goal's OPENING_ASK_CAP counter.

plugins/session-flow/scripts/save_point.py#L607-L620

The PR's stated goal is "one definition of the section boundary rather than two that can drift," and it achieves that between the goal_lines skip (line 593) and build_skeleton's kept skip (line 1103) by routing both through _ends_opening_ask. But the hop-1 ask-length counter three lines below still uses the old, narrower boundary directly:

for line in body[ask_idx + 1 :]:
    if line.startswith("**"):
        break
    if line.strip():
        block += 1

Before this PR, that was consistent with the (also **-only) skip it sits next to. After this PR, it is not: if a hop-1 file is amended in place — the exact scenario this PR fixes the carry for, an amendment bullet added directly below hop-1's own Opening ask: line — and someone runs validate <hop1-file> --strict-transcript before hop 2 exists, this loop keeps counting past the bullet (since it only breaks on **) and folds the amendment's line(s) into the reported "ask length," while the goal_lines skip three lines above now correctly excludes that same bullet from the goal quote. The two loops in the same function disagree about where the ask ends.

Impact is soft — f.warn, not f.fail, so it can't fail --strict-transcript — but it can produce a misleading Original goal: 'Opening ask:' runs N lines (cap 15) warning that blames the verbatim ask for length actually contributed by an amendment bullet. Since _ends_opening_ask already exists and is exported for exactly this purpose, the natural fix is if _ends_opening_ask(line): break here too. The new test doesn't exercise this path: it only validates hop 2 (whose Opening ask: line is a see … pointer, so hop == 1 never triggers on it) and never runs validate on the amended hop-1 file directly.

Fix this →

Everything else checked, no issues

• _ends_opening_ask's two call sites (save_point.py:593 and save_point.py:1103) are byte-consistent with the PR's claim, verified by reading both in full.
• BULLET_RE (^(?:[-*+]|\d+[.)])\s+) correctly matches the amendment bullet's - **Amended (verbatim, …)** form, and doesn't collide with the pre-existing **Amended:** (non-bulleted) skeleton marker handling a few lines below.
• Version bump and CHANGELOG.md entry are consistent with the repo's convention; plugin.json carries no other stale reference.
• Author-claimed verification (pytest run, affected-tests.sh, hop_chain.test.sh, validate-plugins.sh, run-ruff.sh) is described in the PR body but not independently re-run here — noting per lane convention that it's author-claimed, not confirmed by this review.

Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count.

PR body contract — issue linkage

This PR body conforms to the issue-linkage contract. Nothing to do.

Review-lane follow-up: taken. The hop-1 OPENING_ASK_CAP counter in _check_original_goal now breaks on _ends_opening_ask(line) too (619cd0d), so all three reads of the section share one boundary instead of two agreeing and one drifting. The CHANGELOG entry says so. No test added for that path: the cap warn has no coverage before or after this change, and the brief for this PR is one regression case; the PR body records it as reasoned, not pinned.

The Codex P2 thread on the bullet terminator is answered inline and resolved: a narrower - **Amended match would still drop the non-bold parenthetical note bullets that appear in the real reproducer files, and over-carrying visible ask prose in a contract-violating case beats silently deleting operator-stated amendments.